Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reserve promise in top level module #6631

Merged
merged 6 commits into from
Jan 27, 2016
Merged

Conversation

rbuckton
Copy link
Member

This address #6068.

We have chosen to more closely align with ES2016 (ES7) and restrict the return types of async functions in ES6 (and later) to only return instances of the global Promise object. This has the following ramifications:

Return Types

If an async function or method has a return type, it must be a type reference to global generic Promise type. This aligns with the same return type requirements of C#.

User-supplied Promise constructors

We will no longer instantiate a user-supplied Promise constructor during the initialization of an async function. Allocation of the Promise still happens inside of the __awaiter helper, so users are encouraged to define their own __awaiter helper if they need to use a custom Promise. We may still support user-supplied Promise constructors for ES5/3 when if/when we add support for generators and async functions via further transpilation, as these targets do not support a native Promise implementation per their specifications.

Promise reserved at the top-level of a module

We now reserve the identifier Promise for any declarations at the top-level of a module containing async functions or methods. This aligns with how we already reserve require and exports at the top-level of a module. Any user today that currently imports Promise from an external module that wishes to use async functions or methods in their codebase will need to rename or alias the local Promise declaration to avoid this error.

For example:

import * as Promise from "bluebird";
// error TS2529: Duplicate identifier 'Promise'. Compiler reserves name 'Promise' in the top level scope of a module containing async functions.

export async function fn() {
  await Promise.delay(10);
}

It is still acceptable to reference Promise in an expression, so this is perfectly acceptable:

async function fn() {
  await Promise.resolve(1); // 
}

@@ -1,12 +1,8 @@
tests/cases/conformance/async/es6/functionDeclarations/asyncFunctionDeclaration15_es6.ts(6,16): error TS1055: Type '{}' is not a valid async function return type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this error be removed? it seems that this error The return type of an async function or method must be the global Promise<T>. replaces it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors will remain as we are keeping the logic that uses them for use with downlevel async functions for ES5 and earlier.

@yuit
Copy link
Contributor

yuit commented Jan 26, 2016

👍 after some comments

@@ -195,6 +195,10 @@
"category": "Error",
"code": 1063
},
"The return type of an async function or method must be the global Promise<T>.": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global 'Promise<T>' type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add "type". This was paraphrased from the following error message in C#:

The return type of an async method must be void, Task, or Task<T>.

@@ -12495,6 +12509,36 @@ namespace ts {
}

/**
* Checks that the return type provided is an instantiation of the global Promise<T> type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the compiler to reject type annotations that are known compatible with Promise? This won't catch any type errors, but rejects valid annotations. Generator functions handle this consistently IMO. Why not use the same approach here for consistency?

function* bar() {}                  // Returns IterableIterator<any>
function* bar2(): any {}            // OK: compatible annotation
function* bar3(): Iterator<any> {}  // OK: comtatible annotation
function* bar4(): { next } {}       // OK: comtatible annotation

async function foo() {}                     // Returns Promise<void>
async function foo1(): any {}               // ERROR with this PR, but why?
async function foo2(): PromiseLike<any> {}  // ERROR with this PR. but why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checker.ts code for generator functions uses checkTypeAssignableTo to check the return type annotation (L11625). Why is checkTypeAssignableTo not used for async functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the result of an offline discussion with @mhegazy. Supporting an assignable interface is not currently compatible with our goals for future down-level support for async functions for ES5. As a result, async functions will for the time being be more restrictive than generators with respect to the return type annotation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES3/5 restrictions need not be applied to ES6 code. Why not this:

if (languageVersion >= ScriptTarget.ES6) {
    // use checkTypeAssignableTo to check return type annotation
}
else {
    // more restrictive return type annotation rules for ES3/5...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhegazy you gave qualitied support back in December for treating return type annotations one way for ES3/5 (improve the error message), and another way for ES6+ (use checkTypeAssignableTo). Is this still the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current support we have in TypeScript 1.7.x for async functions allows you to specify a custom Promise implementation in the return type annotation. This was intended to support runtimes that do not yet define a global Promise implementation, and was primarily intended to support down-level emit for async functions in ES5, which is a goal for TypeScript 2.0. This allows you to write an async function in the following fashion:

import * as bluebird from 'bluebird';

export async function fn(): bluebird.Promise {
}

console.log(fn() instanceof bluebird.Promise); // true 

However, this approach is not compatible with the ES7 approach to async functions which only leverages the native Promise implementation. As this native Promise is specified as part of ES6, we have elected to only allow you to supply a global Promise<T> return type for ES6, but reserve the previous functionality to eventually support async functions down-level.

If we were to then allow you to specify the return type of an async function as any type to which Promise<T> is assignable, then the above sample would still compile successfully, due to how "bluebird.d.ts" is currently implemented on DefinitelyTyped. The result is that upgrading from TypeScript 1.7.x to TypeScript 1.8 would result in completely different runtime semantics with no indication to the developer that anything had changed.

import * as bluebird from 'bluebird';

export async function fn(): bluebird.Promise {
}

console.log(fn() instanceof bluebird.Promise); // false?! 

By restricting the return type annotation to only the global Promise<T> type, we have the ability to loosen this restriction in the future. Yes, this does not fully align with how we support type annotations on generator functions; however, this is the only reliable way forward. We may, in a future release, opt to add a different mechanism for defining a compatible Promise implementation for ES5 async functions. If that does indeed become the case, we would be able to loosen the return type restriction.

As this is a breaking change from TypeScript 1.7 behavior, it also is best to be more conservative for one to two releases to ensure any existing implementations have adjusted to this change before examining the feasibility of relaxing this restriction.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the return type must be strictly equal to Promise might become a hindrance in scenarios involving interfaces and/or methods overriding.

You can work around it by adding a layer of indirection (like all CS problems 😉) but it doesn't feel nice.

Here's one example: in frameworks such as Aurelia, many methods can return a value or a Promise for a value. Not mandating a Promise is both convenient when implementing trivial methods (like canSave() { return true; }) and good for perf.

Say I have a base class that exposes such a function and is meant to be overriden. Assume that the base default implementation is async.

The natural way is this:

class BaseViewModel {
  async isValid() : boolean | Promise<boolean> { 
    /* some async implementation, maybe server call */  
  }
}

class DerivedViewModel extends BaseViewModel {
  isValid() : boolean | Promise<boolean> {
    return true;  // Non-async implementation
  }
}

but this wouldn't work :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jods4 good real world example. Current behaviour:

var q: () => boolean | Promise<boolean>;
q = () => true;                                          // OK
q = () => Promise.resolve(true);                         // OK
q = async () => true;                                    // OK
(): boolean | Promise<boolean> => true;                  // OK
(): boolean | Promise<boolean> => Promise.resolve(true); // OK
async (): boolean | Promise<boolean> => true;            // ERROR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jods4 since this PR is done and closed may I suggest taking your example across to #6686 where I think it would be quite relevent.

@DanielRosenwasser
Copy link
Member

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants